Add the ability to inject authenticators. #2311
Conversation
k8s-ci-robot
added
approved
| @@ -65,6 +65,12 @@ export class KubeConfig implements SecurityAuthentication { | |||
| new OpenIDConnectAuth(), | |||
| ]; | |||
|
|
|||
| // Optionally add additional external authenticators, you must do this | |||
| // before you load a kubeconfig file that references them. | |||
There was a problem hiding this comment.
Do you think it is worth adding a test that intentionally does not do this (if for no other reason than knowing if it accidentally changes)?
There was a problem hiding this comment.
I don't think this can ever change, because the kubeconfig file references the authenticator as part of the YAML file, so if we don't know about the authenticator because it hasn't been registered when we load the kubeconfig, there's no way that we can link the authenticator requested in the file to the kubeconfig.
So while we could add a test, I'm not sure it would ever break.
src/config.ts
Outdated
| // Optionally add additional external authenticators, you must do this | ||
| // before you load a kubeconfig file that references them. | ||
| public static addAuthenticator(authenticator: Authenticator): void { | ||
| this.authenticators.push(authenticator); |
There was a problem hiding this comment.
i may be misunderstanding typescript classes here, but is the intended use here to add them to all existing KubeConfig objects since the authenticators property is static?
also, could we avoid the loading order issue by adding an optional parameter for an array of authenticators to the constructor instead? that way the authenticators must be added prior to loading a config
There was a problem hiding this comment.
I'm ok with this, but I also think it makes it a little more complicated. I wanted the authenticators to be able to self-register just by being import-ed which requires the static registration. @cjihrig wdyt?
There was a problem hiding this comment.
One thing to note is that the authenticators are already static, so I think making this method static aligns well with the current setup.
I don't personally see a ton of upside to self-registering via import - my understanding is that it would mean calling KubeConfig.addAuthenticator() inside the import instead of in the code managing the KubeConfig instance.
Given the choice between static and per-instance custom authenticators, I think per-instance is likely to have less surprising behavior. For example, in the test added in this PR, wouldn't CustomAuthenticator be included in every subsequent test?
static ones.
|
Ok, I switched this to custom authenticators that are local to the Please take another look. |
|
LGTM. I'll give @davidgamero a chance to weigh in before merging. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anandfresh, brendandburns, cjihrig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #2263